pkg/sql: use ring.Buffer in StmtBuf #41379
Merged
+154
−102
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit cleans up and extends the ring buffer package. In addition to improving testing and making the code more uniform, it makes the following changes: - adds a Cap method - adds a Reserve method - starts the buffer with a capacity of 1 instead of jumping up to 8. This makes the buffer optimal when the maximum size it ever reaches is 1. This mirrors how Go slices grow, for the same reason. Release justification: None. Don't merge. Release note: None
The StmtBuf has the exact access patterns typically associated with a ring buffer. Command instances are added to the back of the buffer and trimmed from the front of the buffer. These operations are often performed in lockstep, but that is not always the case, so we need the buffer to be able to grow. `ring.Buffer` provides exactly these semantics and it allows us to avoid memory allocations on each Command in `StmtBuf.Push`. Release justification: None. Don't merge until branch is split. Release note: None
|
This change is |
andreimatei
reviewed
Oct 9, 2019
There was a problem hiding this comment.
LGTM but I don't understand exactly what allocation we're avoiding. Could you edify me?
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)
|
The key is that the old approach had a slice that it was appending to and truncating from the front. This resulted in behavior analogous to the following: func BenchmarkAppendTruncate(b *testing.B) {
var a []int
for i := 0; i < b.N; i++ {
a = append(a, 1)
a = a[1:]
}
}
// BenchmarkAppendTruncate-16 100000000 16.3 ns/op 8 B/op 1 allocs/opThe slice oscillates between a length of 0 and 1 and has to allocate on each append. |
|
bors r+ |
|
bors crash: bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Oct 10, 2019
41356: Revert "opt: disallow mutations under union" r=RaduBerinde a=RaduBerinde This reverts commit a34d705. Release justification: recently merge fix no longer needed (thanks to #41307). Release note (sql change): Mutations under UNION or UNION ALL are allowed again. 41358: sql/logictest: add logictest for all expected 1PC txn statements r=nvanbenschoten a=nvanbenschoten First commit from #41324. We expect a selection of simple single-statement mutations to hit the "1-phase commit" transaction fast-path. #41320 demonstrated how critical this is, as regressions here can cause major (> 50%) performance hits to benchmarks and user workloads. This commit adds a logic test to validate that these statements continue to hit this fast-path. Release justification: Testing only. 41371: kv: avoid allocations in client.Txn constructor r=nvanbenschoten a=nvanbenschoten This PR contains two small wins that help speed up the client.Txn constructor, which is responsible for **2.90%** of a CPU profile when running Sysbench's `oltp_point_select` workload. The biggest win here will come from addressing #32508. #### kv: lazily create *RangeIterator in txnPipeliner This allocation was responsible for **0.34%** of a CPU profile when running Sysbench's `oltp_point_select` workload. #### kv: only re-alloc refresh spans in augmentMetaLocked if necessary This was responsible for **0.057%** of a CPU profile when running Sysbench's `oltp_point_select` workload. Release justification: None. These can wait for the branch to split. 41372: sql/pgwire: statically allocate format code slice for all text args/cols r=nvanbenschoten a=nvanbenschoten This allocation was responsible for **0.8%** of a CPU profile when running Sysbench's oltp_point_select workload. Release justification: Probably none, although this does look very safe. Release note: None 41379: pkg/sql: use ring.Buffer in StmtBuf r=nvanbenschoten a=nvanbenschoten The StmtBuf has the exact access patterns typically associated with a ring buffer. Command instances are added to the back of the buffer and trimmed from the front of the buffer. These operations are often performed in lockstep, but that is not always the case, so we need the buffer to be able to grow. `ring.Buffer` provides exactly these semantics and it allows us to avoid memory allocations on each Command in `StmtBuf.Push`. Release justification: None. Don't merge until branch is split. Co-authored-by: Radu Berinde <radu@cockroachlabs.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Build succeeded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The StmtBuf has the exact access patterns typically associated with a ring buffer. Command instances are added to the back of the buffer and trimmed from the front of the buffer. These operations are often performed in lockstep, but that is not always the case, so we need the buffer to be able to grow.
ring.Bufferprovides exactly these semantics and it allows us to avoid memory allocations on each Command inStmtBuf.Push.Release justification: None. Don't merge until branch is split.